fix: make StartConditions sync, drop block_in_place under ctrl_lock by YuanYuYuan · Pull Request #2779 · eclipse-zenoh/zenoh

YuanYuYuan · 2026-09-09T10:45:43Z

Summary

Fixes #2581. Under peer churn, the RX thread can deadlock the whole session.

The RX thread holds ctrl_lock. It calls block_in_place to wait on StartConditions, which is a tokio::sync::Mutex. At the same time, the Net runtime's single worker thread tries to acquire that same ctrl_lock, from Router::new_transport_unicast during gossip autoconnect. The worker cannot run. So the task that would release StartConditions never runs either. The session freezes.

This PR is a companion to #2637. #2637 fixes the same root cause with a different mechanism. See "Relationship to #2637" below.

The deadlock

RX thread: holds ctrl_lock. Calls block_in_place on StartConditions. Two call sites: gossip.rs's link_states tail, and the peer hat's route_declare_final.
Net worker (the runtime's only worker thread): blocked separately, trying to acquire ctrl_lock from Router::new_transport_unicast during gossip autoconnect.
Nothing releases StartConditions, because the one worker that could run is stuck on ctrl_lock. The session wedges: puts freeze, transport leases die.

The fix

Every StartConditions critical section is a plain Vec push or drain. None of them await anything. So peer_connectors becomes a std::sync::Mutex instead of a tokio::sync::Mutex. Every method on StartConditions becomes synchronous. Both block_in_place call sites are deleted, not rescheduled.

Relationship to #2637

#2637 takes a different approach at the same two sites: spawn the notification as a task on ZRuntime::Net, instead of blocking on it. That removes the RX thread from this specific cycle. But the spawned task still needs ZRuntime::Net's one worker to run. Nothing in that fix adds a second worker, and nothing removes the single-worker precondition that caused the deadlock. If that worker is ever blocked elsewhere, the spawned task queues indefinitely. If a future caller blocks synchronously on the task's result, the same deadlock returns, with one extra hop.

This PR removes the scheduling dependency instead. There is no .await left inside StartConditions for anything to be rescheduled onto.

@otamachan's independent reproduction on #2637 goes through rmw_zenoh_cpp and ROS 2 directly, with a full five-thread backtrace. It confirms both #2637's approach and this PR's approach target the same two block_in_place sites. Worth reading alongside this PR.

Credit

This PR ports a fix independently developed and field-validated by Guillaume Doisy (Dexory): botsandus/zenoh@ff6cc2bc3. Authorship is preserved on the commit (Author: Guillaume Doisy <guillaume@dexory.com>). Their original commit message, quoted for the record:

Seen in production as random ROS 2 process freezes on 16-25% of bringups.

This PR ports that fix onto current main. It adapts it past the #2096 hat rename (p2p_peer to peer) and the resulting route_declare_final signature change. No other logic changed.

Testing

cargo fmt --check -p zenoh: clean
cargo clippy -p zenoh --lib -- -D warnings: clean. Five pre-existing warnings remain in unrelated files, untouched by this diff.
cargo test -p zenoh --lib net::runtime::orchestrator: 3 of 3 pass
No new test covers the deadlock itself. The field evidence cited above already covers it at a scale (about 1,700 restart cycles, plus an independent 200Hz A/B test) that a unit test here cannot easily reproduce. Happy to add a regression test if maintainers want one in a specific shape.

The RX thread handles OAM/Declare messages holding ctrl_lock and the tables write lock, then parks in ZRuntime::Net.block_in_place(...) waiting on the StartConditions tokio mutex (two sites: the gossip link_states tail and the peer hat's route_declare_final). The single Net worker meanwhile blocks on ctrl_lock in autoconnect's transport setup, and tokio's fair mutex handoff can strand the permit in a Net task that never gets polled again -- deadlocking the whole session. Every StartConditions critical section is a pure Vec operation that never awaits, so its tokio::sync::Mutex becomes std::sync::Mutex, all its methods go sync, and both block_in_place sites are deleted outright. Ported onto current main from a fix independently developed and field-validated by Guillaume Doisy (Dexory), adapting it past the eclipse-zenoh#2096 hat rename (p2p_peer -> peer) and the resulting route_declare_final signature change. Original work, full credit preserved via authorship. See PR body for the upstream discussion this consolidates.

codecov · 2026-09-09T11:30:05Z

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.76%. Comparing base (b828c6c) to head (49c2dd9).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
zenoh/src/net/protocol/gossip.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
+ Coverage   74.72%   74.76%   +0.04%     
==========================================
  Files         419      419              
  Lines       63955    63934      -21     
==========================================
+ Hits        47790    47803      +13     
+ Misses      16165    16131      -34

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

doisyg · 2026-09-09T14:09:58Z

ECA validated

YuanYuYuan · 2026-09-10T15:34:12Z

ECA validated

@doisyg Thanks!

@JEnoch Could you please review this?

JEnoch

LGTM but I would like also @OlivierHecart to review, as he's the architect of this code part.

fuzzypixelz

A less intruisive fix is possible here; I don't have a good understanding of the consequences of switching StartConditions::peer_connectors from tokio::sync::Mutex to std::sync::Mutex. Instead we could leverage tokio::sync::Mutex::blocking_lock:

pub(crate) fn terminate_peer_connector_zid_blocking(&self, zid: ZenohIdProto) {
        let mut peer_connectors = self.peer_connectors.blocking_lock();
        if let Some(peer_connector) = peer_connectors.iter_mut().find(|pc| pc.zid == Some(zid)) {
            peer_connector.terminated = true;
        } else {
            peer_connectors.push(PeerConnector {
                zid: Some(zid),
                terminated: true,
            })
        }
        if peer_connectors.iter().all(|pc| pc.terminated) {
            self.notify.notify_one()
        }
    }

The catch is that we should be careful not to call blocking_lock in an async context.

I took the same escape hatch in order to eliminate block_in_place in #2442 and solve a similar yet different problem.

I've triggered a CI run for this alternative solution here.

fuzzypixelz · 2026-09-11T13:59:15Z

The catch is that we should be careful not to call blocking_lock in an async context.

So I was wrong here (see the CI run), obviously zenoh message handling in general occurs on RX. Making this pattern work would require wrapping all of read_messages in spawn_blocking here—leading to a much more invasive patch that might hurt throughput:

zenoh/io/zenoh-transport/src/unicast/universal/link.rs

Lines 518 to 535 in 52ae1e7

    
               loop { 
        
                   tokio::select! { 
        
                       batch = read(link, priority, pool) => { 
        
                           let batch = batch?; 
        
                           lease_tracker.reset(); 
        
                           #[cfg(feature = "stats")] 
        
                           { 
        
                               let header_bytes = if link.link.is_streamed() { 2 } else { 0 }; 
        
                               stats.inc_bytes(zenoh_stats::Rx, header_bytes + batch.len() as u64); 
        
                           } 
        
                           transport.read_messages(batch, link, #[cfg(feature = "stats")] &stats)?; 
        
                       } 
        
                       _ = lease_tracker.wait_if(priority.unwrap_or(Priority::Control) == Priority::Control) => { 
        
                           bail!("{link}: expired after {} milliseconds", lease_tracker.timeout().as_millis()); 
        
                       } 
        
                   } 
        
               } 
        
           }

You can see here that the read_loop future is calling reading_messages which calls into synchronous routing code; to me that's where the weak point really is.

The remaining question is whether a deadlock is possible where NET or RX is waiting on StartConditions::peer_connectors while the a zenoh runtime is holding it and waiting on the other zenoh runtime (there are still numerous ZRuntime::block_in_place calls).

In any case, I don't see a simpler patch here. LGTM.

YuanYuYuan added the bug Something isn't working label Sep 9, 2026

YuanYuYuan marked this pull request as ready for review September 9, 2026 11:02

YuanYuYuan requested a review from JEnoch September 10, 2026 15:33

JEnoch requested a review from OlivierHecart September 10, 2026 16:25

JEnoch approved these changes Sep 10, 2026

View reviewed changes

OlivierHecart approved these changes Sep 11, 2026

View reviewed changes

fuzzypixelz self-requested a review September 11, 2026 11:43

fuzzypixelz reviewed Sep 11, 2026

View reviewed changes

fuzzypixelz approved these changes Sep 11, 2026

View reviewed changes

JEnoch merged commit 646f2d1 into eclipse-zenoh:main Sep 11, 2026
34 of 35 checks passed

JEnoch mentioned this pull request Sep 11, 2026

fix: don't park the ctrl_lock holder on StartConditions notifications under peer churn #2637

Open

6 tasks

Navigation Menu

Uh oh!

fix: make StartConditions sync, drop block_in_place under ctrl_lock - #2779

fix: make StartConditions sync, drop block_in_place under ctrl_lock#2779
JEnoch merged 1 commit into
eclipse-zenoh:mainfrom
YuanYuYuan:fix/ctrl-lock-block-in-place

YuanYuYuan commented Sep 9, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Sep 9, 2026

Uh oh!

doisyg commented Sep 9, 2026

Uh oh!

YuanYuYuan commented Sep 10, 2026

Uh oh!

JEnoch left a comment

Uh oh!

fuzzypixelz left a comment •

edited

Loading

Uh oh!

fuzzypixelz commented Sep 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

YuanYuYuan commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The deadlock

The fix

Relationship to #2637

Credit

Testing

Uh oh!

codecov Bot commented Sep 9, 2026

Codecov Report

Uh oh!

doisyg commented Sep 9, 2026

Uh oh!

YuanYuYuan commented Sep 10, 2026

Uh oh!

JEnoch left a comment

Choose a reason for hiding this comment

Uh oh!

fuzzypixelz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuzzypixelz commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YuanYuYuan commented Sep 9, 2026 •

edited

Loading

fuzzypixelz left a comment •

edited

Loading

fuzzypixelz commented Sep 11, 2026 •

edited

Loading